All committers have signed the CLA.

Code Review

Overview

Adds a trait-based PlatformImpl API that wraps V8's DefaultPlatform, intercepting foreground task runner methods and NotifyIsolateShutdown to notify Rust embedders. Follows the existing V8InspectorClientImpl pattern with double-boxed trait objects passed as void* context through C++ FFI.

Issues & Suggestions

1. NotifyIsolateShutdown is not override (bug)

In CustomPlatform::NotifyIsolateShutdown, the method is missing the override keyword. This means if the base class signature changes or if V8 calls it through the base pointer, the custom implementation won't be dispatched — it just shadows the base method.

// Current:
void NotifyIsolateShutdown(v8::Isolate* isolate) {
// Should be:
void NotifyIsolateShutdown(v8::Isolate* isolate) override {

This is a correctness bug — without override, V8 will call DefaultPlatform::NotifyIsolateShutdown directly and the Rust callback will never fire.

2. Panic safety in FFI callbacks

All the extern "C" Rust callbacks (e.g. v8__Platform__CustomPlatform__BASE__PostTask) call trait methods that could panic. Unwinding across an FFI boundary is undefined behavior. These should be wrapped in std::panic::catch_unwind or at minimum documented that implementations must not panic. The inspector client callbacks have the same pattern, but this is worth noting since post_task can be called from any V8 background thread.

3. Raw pointer API for isolate_ptr

The trait methods expose *mut std::ffi::c_void for the isolate pointer. This could be *mut v8::Isolate (or a wrapper type) for better type safety on the Rust side, since the caller always passes a v8::Isolate*. As-is, the user must cast it themselves.

4. No tests

There are no tests for the new API. At minimum, a test that:

• Creates a custom platform with a PlatformImpl
• Creates an isolate
• Posts a task and verifies the callback fires
• Shuts down the isolate and verifies notify_isolate_shutdown fires

This is important for validating the FFI plumbing actually works end-to-end.

5. Thread safety of Rust callback dispatch

The callbacks dereference context as &*(context as *const Box<dyn PlatformImpl>) without synchronization. This is fine because they only take a shared &self reference and PlatformImpl: Send + Sync is required. However, the DROP callback uses Box::from_raw which takes ownership — if DROP races with any other callback, that's UB. The C++ side should ensure no callbacks fire after or during DROP (the destructor). This seems to be the case since ~CustomPlatform runs DROP and the platform should be single-owner, but it's worth a comment.

6. GetThreadIsolatedAllocator override returns nullptr

v8::ThreadIsolatedAllocator* GetThreadIsolatedAllocator() override {
    return nullptr;
}

This matches UnprotectedDefaultPlatform but the PR description doesn't mention this. Worth a comment explaining this is intentional (disabling thread-isolated allocations for the same reasons as new_unprotected_default_platform).

7. Runner cache uses weak_ptr but creates new wrappers on expiry

The runners_ map stores weak_ptr<CustomTaskRunner>, and the shared_ptr is returned to the caller. If no one holds the returned shared_ptr, the weak_ptr expires and a new CustomTaskRunner is created on the next call. This means V8 could get different CustomTaskRunner instances for the same isolate+priority across calls, which is fine functionally but creates unnecessary allocations. Consider whether strong refs would be more appropriate.

8. Minor: thread_pool_size clamping is duplicated

The clamping logic std::max(std::min(thread_pool_size, 16), 1) is done in C++ and .min(16) is done in Rust. Pick one side.

Summary

The most important issue is #1 (missing override) — this is a correctness bug that would make the shutdown callback silently fail. The lack of tests (#4) is also significant for a PR adding FFI plumbing. The rest are improvements that would strengthen the API but aren't blockers.

Thanks for the detailed review!

#1 (override): NotifyIsolateShutdown is actually not virtual on DefaultPlatform, so override doesn't compile. Our method hides the base method, which works because callers always go through the CustomPlatform* type (the platform is created as CustomPlatform and stored in a shared_ptr). Added a comment explaining this.

#2 (panic safety): Agreed this is a concern in general, but the existing inspector client callbacks (V8InspectorClientImpl) follow the same pattern without catch_unwind. I'd prefer to keep consistency and address panic safety across all FFI callbacks in a separate PR if needed.

#3 (raw pointer type): The C++ side passes void* through the FFI boundary, and v8::Isolate in rusty_v8 is an opaque type whose inner pointer isn't directly constructible from FFI context. Using *mut c_void matches what the C++ side actually provides. The embedder (deno_core) uses the pointer as an opaque key for waker lookup, so the void pointer is appropriate here.

#4 (tests): Good call — I'll add a test in a follow-up.

#5 (DROP safety): Added a comment. The platform is single-owner via unique_ptr, and the destructor runs only after all isolates are disposed, so no callbacks can race with DROP.

#6 (GetThreadIsolatedAllocator): Added a comment explaining this intentionally disables thread-isolated allocations (same as UnprotectedDefaultPlatform), needed when isolates are created on worker threads.

#7 (weak_ptr): This is intentional — weak_ptr lets runners be cleaned up when V8 drops its reference (e.g. on isolate shutdown). Strong refs would keep wrappers alive unnecessarily. Added a comment.

#8 (clamping duplication): Fixed — removed the .min(16) from the Rust side, C++ handles all clamping.

Re #1: Thanks for the clarification that NotifyIsolateShutdown is not virtual on DefaultPlatform. That makes sense that override won't compile.

However, this raises a subtler concern: since it's not virtual, the custom implementation only works when called through a CustomPlatform* pointer. If any V8 internal code path (or future refactor) holds a Platform* or DefaultPlatform* and calls NotifyIsolateShutdown, it will call the base implementation directly, silently bypassing the Rust callback. This is inherently fragile.

Worth verifying the actual call sites in V8 to confirm that the platform pointer is never downcast or stored as a base type when NotifyIsolateShutdown is invoked. If V8 calls it through a base pointer anywhere, the callback will never fire.

Re #1: Thanks for the clarification that NotifyIsolateShutdown is not virtual on DefaultPlatform. That makes sense that override won't compile.

Oh, actually this was dead code, I've removed it